[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492
Open
csun5285 wants to merge 7 commits into
Open
[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492csun5285 wants to merge 7 commits into
csun5285 wants to merge 7 commits into
Conversation
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed with one inline issue.
Critical checkpoint conclusions:
- Goal/refactor: the RowKeyEncoder extraction preserves the old writer-local full-key, short-key, sequence-suffix, cluster-key primary-key separation, and rowid-suffix flows based on static comparison with GitHub base snapshots.
- Scope: the production changes are focused in BE storage key/segment writer paths; I did not find another substantiated production correctness issue.
- Concurrency/lifecycle: no new shared mutable state, thread entry, lock ordering, or special lifetime boundary was introduced by the helper; it stores schema-derived coder pointers like the old writers did.
- Config/compatibility/persistence: no config, protocol, edit-log, or storage-format change was found; the encoded byte layout is covered by new unit-test goldens.
- Parallel paths: SegmentWriter, VerticalSegmentWriter, and BlockAggregator call paths were checked, including cluster-key and sequence-column paths.
- Tests/style: one test hygiene issue remains: the new unit test leaks
private/protectedmacros into later includes. - User focus: no additional user-provided focus was present.
Validation was static only: this checkout is missing .worktree_initialized, thirdparty/installed, thirdparty/installed/bin/protoc, and clang-format v16, and the base commit is not present locally. I used GitHub PR metadata/base snapshots for diff scope. git show --check reported no whitespace errors on the changed files.
Subagent conclusions: tests-session-config proposed TSC-001, accepted as MAIN-001 and submitted inline. optimizer-rewrite found no candidates. Convergence round 1 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the final one-comment ledger set.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary:
I found one test-coverage issue in the new RowKeyEncoder unit test. The production refactor itself looks byte-compatible with the removed SegmentWriter/VerticalSegmentWriter helper logic after checking full-key, short-key, null, sequence, rowid, cluster-key MoW, partial-update reachability, min/max, and lookup-side consumers.
Critical checkpoint conclusions:
- Goal/test proof: the refactor has focused unit coverage for marker layout, suffixes, short keys, and many key coders, but the cluster-key separation test does not prove the primary-vs-sort coder split.
- Scope: the implementation is focused on extracting key encoding into RowKeyEncoder and replacing the writer-local helpers.
- Concurrency/lifecycle/config/persistence/FE-BE protocol: no new concurrency, dynamic config, persistence format, or protocol surface was introduced.
- Parallel paths: SegmentWriter and VerticalSegmentWriter were both updated; partial-update and lookup-side paths were checked for compatibility.
- Data correctness: no production key byte-layout or accessor-routing bug was substantiated.
- Tests: one targeted test needs strengthening; no build/tests were run because the review prompt explicitly restricts this task to review operations.
- User focus: no additional user-provided review focus was supplied.
|
|
||
| // With cluster keys the segment sorts by the cluster key columns, while the | ||
| // primary key index is still built over the schema key columns. | ||
| TEST_F(RowKeyEncoderTest, ClusterKeySortDiffersFromPrimaryKeys) { |
Contributor
There was a problem hiding this comment.
This test does not actually catch the primary-vs-cluster coder mixup that the refactor is trying to protect against. cluster_key_schema() makes both the primary key and the cluster key INT columns, and this assertion only proves that encoded value 5 differs from encoded value 99. If full_encode_primary_keys() accidentally used the cluster/sort-key coder vector, the test would still pass because the supplied primary-key accessor is also INT. Please make this case heterogeneous, for example INT primary key plus CHAR/VARCHAR cluster key, and assert the exact hex for full_encode_primary_keys(primary_keys), full_encode(sort_keys), and the short key so a wrong coder vector fails deterministically.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29969 ms |
Contributor
TPC-DS: Total hot run time: 180905 ms |
Contributor
ClickBench: Total hot run time: 25.22 s |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review completed with no new inline issues.
Critical checkpoint conclusions:
- Goal/test proof: the RowKeyEncoder extraction centralizes the former writer-local key encoding while preserving full-key, short-key, primary-key, sequence-suffix, and rowid-suffix behavior. The expanded BE unit test/golden now covers primary-vs-cluster key views, all current default-converted key types, null ordering, short-key truncation, sequence suffixes, and rowid suffix boundaries.
- Scope: the implementation is focused on BE storage key encoding and the horizontal/vertical segment-writer call sites.
- Data correctness: no reachable primary-key-vs-cluster-key accessor mismatch was substantiated. Normal MOW cluster-key writers use
full_encode_primary_keys()for primary-index entries and rebuild cluster-key accessors for short/min/max keys; partial-update paths that would be suspicious are guarded before flush for MOW cluster-key partial updates. - Concurrency/lifecycle: no new shared mutable state, thread entry, lock ordering, or special lifetime boundary was introduced.
- Config/compatibility/persistence/protocol: no dynamic config, edit-log, storage-format, or FE/BE protocol change was introduced. Reader and delete-bitmap consumers still strip/decode the same sequence and rowid suffix lengths.
- Parallel paths: SegmentWriter, VerticalSegmentWriter, BlockAggregator partial-update paths, primary-index readers, and delete-bitmap calculator paths were reviewed.
- Tests/style: the two existing review threads appear addressed in the current head: the white-box macros are scoped to the intended include, and the primary-vs-cluster test now uses heterogeneous/reordered views plus byte-level goldens.
- User focus: no additional user-provided review focus was present.
Subagent conclusions: Round 1 Subagent A (production writer), Subagent B (test/golden), and Risk Subagent C (key-view/suffix reachability) all returned NO_NEW_VALUABLE_FINDINGS; the main risk items R1-R3 were dismissed with code evidence.
Validation was static only. I did not run builds or tests because the review prompt explicitly forbids builds in this review environment.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29625 ms |
Contributor
TPC-DS: Total hot run time: 178552 ms |
Contributor
ClickBench: Total hot run time: 24.91 s |
bobhan1
reviewed
Jul 14, 2026
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wkeyword-macro" | ||
| #endif | ||
| #define private public |
Contributor
There was a problem hiding this comment.
These macros are unnecessary: doris_be_test is already compiled with -fno-access-control in be/test/CMakeLists.txt, so this test can access TabletSchema private fields directly. Keeping private/protected redefined while including tablet_schema.h also rewrites access specifiers in its transitive protobuf, phmap, and Doris headers, which can make this translation unit class definitions differ from other TUs. Please remove the pragma/macro block and include tablet_schema.h normally.
bobhan1
previously approved these changes
Jul 14, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
liaoxin01
reviewed
Jul 14, 2026
|
|
||
| RowKeyEncoder::RowKeyEncoder(const TabletSchema& schema, bool mow) { | ||
| for (size_t cid = 0; cid < schema.num_key_columns(); ++cid) { | ||
| _primary_key_coders.push_back(get_key_coder(schema.column(cid).type())); |
Contributor
There was a problem hiding this comment.
_primary_key_coders 和 _seq_coder 看着只有mow表才会，是不mow和非mow分别弄个函数实现的。
liaoxin01
reviewed
Jul 14, 2026
| // 6. Compare all encoded views with the generated byte-level snapshot. | ||
| // Refresh this snapshot with: | ||
| // ./run-be-ut.sh --run --filter=RowKeyEncoderTest.AllKeyTypesTable --gen_out -j 64 | ||
| const char* root = std::getenv("ROOT"); |
Contributor
Author
There was a problem hiding this comment.
yes，run-be-ut.sh 会把 ROOT 稳定设置为仓库根目录，这样无论在哪里运行这个UT 都是OK的
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Jul 15, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65492 Problem Summary: RowKeyEncoder initialized primary-key and sequence coders for table shapes whose writer paths never use them, and the table-driven test exercised primary-index encoding for non-MOW schemas. Split initialization by table mode, keep the separate primary-key and rowid view only for MOW cluster-key tables, and align the test with the SegmentWriter and VerticalSegmentWriter call paths. Regenerate the golden so non-MOW rows report the inapplicable primary-index key as N/A while preserving every MOW encoded key byte. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.* -j64 (3 tests passed) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
github-actions
Bot
removed
the
approved
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review completed with one requested test-coverage fix.
Critical checkpoint conclusions:
- Goal fit: the RowKeyEncoder extraction preserves the intended source behavior in the reviewed writer paths, but the new golden coverage misses a supported cluster-key shape central to this refactor.
- Scope/focus: the implementation is narrowly scoped to storage key encoding and writer call-site cleanup.
- Concurrency/lifecycle: no new shared mutable concurrency, lock ordering, static initialization, or ownership lifecycle issue was found.
- Compatibility/persistence/config: no storage-format, EditLog, FE/BE protocol, or config compatibility change was introduced.
- Parallel paths: SegmentWriter and VerticalSegmentWriter were both updated; MoW cluster-key partial update remains rejected before segment writing, while normal UPSERT uses the primary and cluster key views on the correct paths.
- Tests: existing tests cover byte layout for many types and suffixes, but they still miss cluster-key schemas where primary-key and sort-key accessor counts differ.
User focus: no additional user-provided focus points were supplied.
Validation: static review only, per the review prompt. I did not run builds or tests in this review-only runner.
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Jul 15, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65492 Problem Summary: The RowKeyEncoder golden matrix used equal primary-key and cluster-key accessor counts, so it did not prove that each encoding view consumes its own accessor set. Add a merge-on-write cluster-key case with one primary key, four cluster keys, and a two-column short-key prefix, and pin the full sort key, primary key, short key, and primary-index key bytes. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.* -j64 (3 tests passed) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
…both segment writers
SegmentWriter and VerticalSegmentWriter carry two verbatim copies of the
row-key encoding logic (_full_encode_keys x2, _encode_seq_column,
_encode_rowid, _encode_keys, plus six coder members each). Extract it into
a shared RowKeyEncoder (be/src/storage/key/row_key_encoder.{h,cpp}) with
the two views made explicit:
- full_encode() / encode_short_keys(): the sort-key view (cluster key
columns for mow tables with cluster keys, schema key columns otherwise)
- full_encode_primary_keys(): the primary-key view (always the schema key
columns; the primary key index is built on these even when the segment
sorts by cluster keys)
- append_seq_suffix() / append_rowid_suffix(): the two key suffixes
Both writers now hold a RowKeyEncoder member and _generate_primary_key_index
loses its coder parameter. BlockAggregator's calls into the writer's encode
helpers in partial_update_info.cpp are redirected onto the writer's
_key_encoder (decoupling BlockAggregator from the writer entirely is left
to a follow-up).
Equivalence notes for review:
- SegmentWriter's _full_encode_keys had a null_first parameter that no
caller ever set to false; the encoder always uses the null-first marker.
- The constructor DCHECK(num_sort_key_columns >= num_short_key_columns) is
dropped; both counts now derive from TabletSchema in one place.
- SegmentWriter::set_mow_context had zero callers and is removed.
- _maybe_invalid_row_cache intentionally stays in the writers; moving the
row-cache invalidation is out of scope here.
Tests: row_key_encoder_test.cpp with 30 cases - 13 behavior cases (marker
layout, null-first ordering, sort-key vs primary-key views, short-key
truncation and collision, seq/rowid suffixes) plus a golden byte matrix
that pins the encoded bytes of every legal key-column type (TINYINT,
SMALLINT, INT, BIGINT, LARGEINT, BOOL, DATEV2, DATETIMEV2, DECIMAL32/64/
128I/256, IPV4, IPV6, CHAR, VARCHAR, STRING) in all three views as literal
hex strings, so any coder change or CPU-architecture drift fails loudly.
Every existing MoW / partial-update / pk-index UT now executes through
RowKeyEncoder.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… only Address review: the white-box macros were defined before every include of the test TU, so gtest, standard and other Doris headers were all parsed with rewritten access specifiers. Wrap only storage/tablet/tablet_schema.h (the one header the test needs white-box access to) and #undef right after, following the convention of segment_iterator_limit_opt_test.cpp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TIMESTAMPTZ is a legal key column type (the FE key-type blacklist does not exclude it) with its own registered key coder (raw big-endian uint64), so the golden byte matrix must pin it like the other key types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#64674 Problem Summary: The existing RowKeyEncoder tests spread byte-layout and ordering assertions across 31 narrow cases, making it difficult to verify every production schema path consistently. Consolidate them into one explicit 51-row table spanning all 17 current key types and eight valid schema shapes, add data-driven sequence and rowid suffix coverage, and pin every encoded view in an auto-generated golden file. This keeps behavior unchanged while making sort-key, primary-key, short-key, null, sequence, and rowid contracts reviewable from a single dataset. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.* -j64 (3 tests passed) - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.AllKeyTypesTable --gen_out -j64 (golden regenerated) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#64674 Problem Summary: The data-driven RowKeyEncoder table test used golden-prefixed names for runtime result columns and lacked clear structure across its validation phases. Rename the result containers to describe their contents, remove the redundant fixed-size assertion, and divide the test into six documented stages. The test behavior and generated snapshot remain unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.AllKeyTypesTable -j64 (1 test passed) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65492 Problem Summary: RowKeyEncoder initialized primary-key and sequence coders for table shapes whose writer paths never use them, and the table-driven test exercised primary-index encoding for non-MOW schemas. Split initialization by table mode, keep the separate primary-key and rowid view only for MOW cluster-key tables, and align the test with the SegmentWriter and VerticalSegmentWriter call paths. Regenerate the golden so non-MOW rows report the inapplicable primary-index key as N/A while preserving every MOW encoded key byte. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.* -j64 (3 tests passed) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65492 Problem Summary: The RowKeyEncoder golden matrix used equal primary-key and cluster-key accessor counts, so it did not prove that each encoding view consumes its own accessor set. Add a merge-on-write cluster-key case with one primary key, four cluster keys, and a two-column short-key prefix, and pin the full sort key, primary key, short key, and primary-index key bytes. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=RowKeyEncoderTest.* -j64 (3 tests passed) - build-support/check-format.sh (passed with clang-format 16) - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (0 warnings) - Behavior changed: No - Does this need documentation: No
csun5285
force-pushed
the
refactor/row-key-encoder
branch
from
2219644 to
e7ecad0
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I completed a static review of this RowKeyEncoder refactor and did not find any new inline findings to add.
Checkpoint conclusions:
- Goal and scope: the change is focused on deduplicating segment-writer key encoding behind
RowKeyEncoder; the normal non-vertical and vertical writer paths still use the primary-key view for cluster-key MoW primary indexes and the sort/cluster-key view for short/min/max keys. - MoW and data correctness: I rechecked the suspicious partial-update path. The local writer code would be unsafe if cluster-key MoW partial update were allowed, but supported FE paths reject or disable that mode and BE
MemTable::_to_block()rejects it before segment writing, so I am not leaving an inline comment for that dormant path. - Storage-format compatibility: sequence suffix, rowid suffix, primary-index key, short-key, and min/max byte layout match the removed writer-local helper behavior.
- Parallel paths: both
SegmentWriterandVerticalSegmentWriterwere checked, along with direct-writer bypasses and lookup-side suffix parsing. - Tests and build integration: the new helper source and BE UT are covered by existing CMake globs, and the generated expected output now covers the primary/cluster/short-key split including mismatched counts. Existing review threads already covered the earlier test-only gaps, so I avoided duplicating them.
- Concurrency, lifecycle, config, FE-BE protocol, persistence, and observability: I did not see new concerns in these categories for this refactor.
Validation was static only; I did not run builds or tests, consistent with the review bundle instructions.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29402 ms |
Contributor
TPC-DS: Total hot run time: 177213 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both writers now hold a RowKeyEncoder member and _generate_primary_key_index loses its coder parameter. BlockAggregator's calls into the writer's encode helpers in partial_update_info.cpp are redirected onto the writer's _key_encoder (decoupling BlockAggregator from the writer entirely is left to a follow-up).
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)